Skip to content

Fix: Enable custom group_id in add_episode MCP tool #486

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

markalosey
Copy link

@markalosey markalosey commented May 13, 2025

Problem:

The add_episode MCP tool failed when a custom group_id string was provided, erroring with Parameter 'group_id' must be of type undefined, got string. This prevented assigning episodes to specific, non-default groups.

Solution:

This branch corrects the handling of the group_id parameter in the add_episode tool, allowing custom string group_ids to be processed successfully.

Verification:

On this branch, after restarting the server:

  1. Adding Episodes: Successfully added episodes with distinct custom group_ids (e.g., project_alpha_fix_test_001, project_beta_fix_test_002).
  2. Searching Nodes:
    • Confirmed search_nodes correctly filters by a single specified group_id.
    • Confirmed search_nodes correctly filters by a list of specified group_ids.

This validates that adding to and searching within custom groups now works as intended.

Note on Search Behavior:

Currently, search_nodes without a specified group_id defaults to the server's main group_id rather than searching all groups. This could be a future enhancement if "search all" is desired for that scenario.


Important

Fixes add_episode tool to correctly handle custom group_id strings and updates related functions for default handling.

  • Behavior:
    • Fixes handling of group_id in add_episode to accept custom strings, defaulting to config.group_id if empty.
    • Updates search_nodes, search_facts, and get_episodes to handle empty group_id as default.
  • Configuration:
    • GraphitiConfig.from_cli_and_env() now sets group_id with precedence: CLI > Env Var > Default.
    • Logs group_id source in initialize_server().
  • Testing:
    • Makefile: Updates test target to load .env.test using python-dotenv.
    • pyproject.toml: Adds python-dotenv-run to dev dependencies.

This description was created by Ellipsis for 7aba34b. You can customize this summary. It will automatically update as commits are pushed.

@danielchalef
Copy link
Member

danielchalef commented May 13, 2025

All contributors have signed the CLA ✍️ ✅
Posted by the CLA Assistant Lite bot.

Copy link
Contributor

@ellipsis-dev ellipsis-dev bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Important

Looks good to me! 👍

Reviewed everything up to 7aba34b in 1 minute and 39 seconds. Click for details.
  • Reviewed 188 lines of code in 3 files
  • Skipped 2 files when reviewing.
  • Skipped posting 5 draft comments. View those below.
  • Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. Makefile:29
  • Draft comment:
    Consider adding a newline at the end of the file to adhere to standard POSIX formatting practices.
  • Reason this comment was not posted:
    Comment was on unchanged code.
2. mcp_server/graphiti_mcp_server.py:1
  • Draft comment:
    This Python file is missing a copyright notice header. Please include the standard license header as required.
  • Reason this comment was not posted:
    Comment was not on a location in the diff, so it can't be submitted as a review comment.
3. mcp_server/graphiti_mcp_server.py:827
  • Draft comment:
    The nested ternary for assigning effective_group_ids in search_nodes is a bit hard to read. Consider refactoring this into multiple statements or adding explicit parentheses for clarity.
  • Reason this comment was not posted:
    Confidence changes required: 70% <= threshold 80% None
4. pyproject.toml:21
  • Draft comment:
    The dependency specification for python-dotenv uses parentheses. Consider using the standard syntax (e.g., 'python-dotenv[cli]>=1.1.0,<2.0.0') to avoid potential issues with package resolution.
  • Reason this comment was not posted:
    Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 20% vs. threshold = 80% While the comment suggests a valid improvement in syntax style, the parenthesized version is actually valid in pyproject.toml and Poetry. Both syntaxes work equally well. The change would be purely cosmetic and doesn't affect functionality. Package managers like pip and poetry handle both formats correctly. I might be underestimating the importance of following standard conventions. Some tools might handle the parenthesized version differently. While consistency is good, there's no evidence of actual issues with the current syntax, and both forms are valid according to PEP 508. The comment should be deleted as it suggests a purely cosmetic change without clear benefits or evidence of problems.
5. pyproject.toml:21
  • Draft comment:
    Typo: The dependency string 'python-dotenv[cli] (>=1.1.0,<2.0.0)' includes an extra space and parentheses compared to the conventional format. Consider changing it to 'python-dotenv[cli]>=1.1.0,<2.0.0' to follow standard dependency specification.
  • Reason this comment was not posted:
    Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 80% While the suggested format is more common, both formats are valid in Poetry. The current format with parentheses is actually clearer for readability. This seems like a purely stylistic preference rather than a correctness issue. There's no strong evidence that this change is necessary or beneficial. The comment might be pointing out a consistency issue, as other dependencies in the file don't use parentheses. Also, some tools might parse these dependencies differently. The current format is valid Poetry syntax, and there's no evidence of any actual problems. The parentheses actually make the version bounds more readable. This comment should be deleted as it suggests a purely stylistic change to valid code, without providing any concrete benefit or fixing any actual issues.

Workflow ID: wflow_FLLstcPM8ZQbXGI0

You can customize Ellipsis by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.

@markalosey
Copy link
Author

I have read the CLA Document and I hereby sign the CLA

danielchalef added a commit that referenced this pull request May 13, 2025
@danielchalef
Copy link
Member

@markalosey Thank you for this contribution! Just a heads up: I won't be able to review the PR for a few weeks. Expect an early June response. Apologies in advance for the delay!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants